feat(developer): kmc convert typesafety 😎 by SabineSIL · Pull Request #15860 · keymanapp/keyman

SabineSIL · 2026-04-20T15:40:59Z

In several places in kmc-convert: keylayout->kmn we did not care about return type null or undefined for function return values or parameters. Therefore we got several warnings about possible return types not being assignable to symbols.

Even though they did not affect the code running correctly they have been specified more clearly in this PR.

@keymanapp-test-bot skip

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Developer
- Keyman Developer - build : all tests passed (no artifacts on BuildLevel "build")
- Compiler Regression Tests - build : all tests passed (no artifacts on BuildLevel "build")
- Keyman Developer (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
- kmcomp.zip - build : all tests passed (no artifacts on BuildLevel "build")
- kmcomp.zip (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
Keyboards
- Test Keyboards - build : all tests passed (no artifacts on BuildLevel "build")

…vert-Typesafety

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety

…vert-Typesafety

…vert-Typesafety # Conflicts: # developer/src/kmc-convert/src/keylayout-to-kmn/keylayout-to-kmn-converter.ts # developer/src/kmc-convert/src/keylayout-to-kmn/kmn-file-writer.ts # developer/src/kmc-convert/test/keylayout-to-kmn-converter.tests.ts # developer/src/kmc-convert/test/kmn-file-writer.tests.ts

…vert-Typesafety

ermshiperete

Looks pretty good despite the many comments.

ermshiperete · 2026-05-22T09:23:43Z

+   * @brief  helper function to check if a specific keyMap index exists in a keyMapSet
+   * neccessary because the amount of  <keyMap index> must correspond to
+   * the amount of <keyMapSelect mapIndex>


It would be better to make this full sentences. If this gets processed by a tool the three lines might get put in the same paragraph removing the line breaks, which then would make it hard to understand.

Suggested change

* @brief helper function to check if a specific keyMap index exists in a keyMapSet

* neccessary because the amount of <keyMap index> must correspond to

* the amount of <keyMapSelect mapIndex>

* @brief Helper function to check if a specific keyMap index exists in a keyMapSet.

* This is neccessary because the amount of <keyMap index> must correspond to

* the amount of <keyMapSelect mapIndex>.

ermshiperete · 2026-05-22T09:24:16Z

+   * @brief  helper function to check if a specific keyMapSelect index exists in a modifierMap
+   * neccessary because the amount of <keyMap index> must correspond to
+   * the amount of <keyMapSelect mapIndex>


Suggested change

* @brief helper function to check if a specific keyMapSelect index exists in a modifierMap

* neccessary because the amount of <keyMap index> must correspond to

* the amount of <keyMapSelect mapIndex>

* @brief Helper function to check if a specific keyMapSelect index exists in a modifierMap.

* This is neccessary because the amount of <keyMap index> must correspond to

* the amount of <keyMapSelect mapIndex>.

ermshiperete · 2026-05-22T10:02:13Z

  behavior: string;
  modifier?: string;
-  outchar?: string;
+  outchar?: string | undefined;


This should not be necessary. outchar? makes the property optional which implies undefined. So if you write outchar?: string; the type of outchar is implicitly string|undefined.

ermshiperete · 2026-05-22T10:23:24Z

+        // in case writeCharacterOrUnicode() returns null, the fallback is empty strings for characterMessage.character
+        // and characterMessage.message. Then versionOutputCharacter could be "" and would be written into the kmn file
+        // as ... > '', producing an invalid kmn rule.


I don't understand this comment. It describes what the code does and that this could result in an invalid kmn rule. Wouldn't we want to do something so that we don't get invalid kmn rules? (in which case I'd expect a TODO or something).

Or does the comment try to explain why we have the if ((outputCharacter !== undefined) && (outputCharacter !== "")) check? Then it should mention outputCharacter which could lead to writeCharacterOrUnicode returning false...

(Additional occurrences of the same code comment above)

ermshiperete · 2026-05-22T10:31:37Z

  *         null in case of an empty string or null or undefined input
  */
-  public writeCharacterOrUnicode(ctr: string, msg: string = ""): MessageCharacter {
+  public writeCharacterOrUnicode(ctr: string, msg: string = ""): MessageCharacter | null {


I think it would be better if writeCharacterToUnicode would always returns MessageCharacter. In the error case it could return { character: '', message: '' }. IMO this would simplify things quite a bit.

ermshiperete · 2026-05-22T11:20:59Z

+    ].forEach(function (values) {
+      it(('should convert "' + values[0] + '"').padEnd(25, " ") + 'to "' + values[2] + '"', async function () {
+        const result = sutW.writeCharacterOrUnicode(values[0] as string, values[1] as string);
+        if (result) {


It would be better to split this into two tests: one set that tests the cases where we get a result, and the other for the cases that return null.

ermshiperete · 2026-05-22T11:21:39Z

    });
+    it(('null should create empty string '), async function () {
+        const result1 = sutW.writeDataRules(null);
+        assert.isTrue(result1 === '');


I'd use

Suggested change

assert.isTrue(result1 === '');

assert.equal(result1, '');

ermshiperete · 2026-05-22T16:28:14Z

    it('run() should throw on unavailable input file name and null output file name', async function () {
      const inputFilename = makePathToFixture('../data/Unavailable.keylayout');
-      const result = sut.run(inputFilename, null);
+      const result = sut.run(inputFilename, undefined);


Since run is async, we'll have to await it:

Suggested change

const result = sut.run(inputFilename, undefined);

const result = await sut.run(inputFilename, undefined);

ermshiperete · 2026-05-22T16:31:21Z

            this.callbacks.reportMessage(ConverterMessages.Error_UnsupportedCharactersDetected({
              keymapIndex: jsonObj.keyboard.keyMapSet[0].keyMap[i]['index'],
-              output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '',
+              output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '' as string,


as string is unnecessary since it's already a string.

Suggested change

output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '' as string,

output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '',

ermshiperete · 2026-05-22T16:35:16Z


        // use of Unicode Character vs Unicode Codepoint;
        // If it`s a ctrl character we print out the Unicode Codepoint else we print out the Unicode Character
        let versionOutputCharacter;


Might be better to initialize this to the empty string, otherwise we could theoretically end up with an uninitialized variable if the if in line 163 evaluates to false.

Suggested change

let versionOutputCharacter = '';

feat(developer): ensure typesafety for 'return null' and others

19ebc62

SabineSIL added this to the A19S27 milestone Apr 20, 2026

SabineSIL requested review from ermshiperete and mcdurdin April 20, 2026 15:40

SabineSIL self-assigned this Apr 20, 2026

SabineSIL added developer/ mac/ linux/ common/ labels Apr 20, 2026

SabineSIL added this to Keyman Apr 20, 2026

SabineSIL added developer/compilers/ feat common/web/ epic-kmc-convert labels Apr 20, 2026

github-project-automation Bot moved this to Todo in Keyman Apr 20, 2026

keymanapp-test-bot Bot changed the title ~~feat/developer/kmc convert typesafety~~ feat/developer/kmc convert typesafety 😎 Apr 20, 2026

SabineSIL added 4 commits April 20, 2026 17:47

feat(developer): remove tests for entire keyboards

30ea904

feat(developer): read() throw error instead of return null

6802ec4

feat(developer): more typesafety + add tests for validate()

9b9cafe

feat(developer): more typesafety

ef1cf18

SabineSIL changed the title ~~feat/developer/kmc convert typesafety 😎~~ feat(developer):kmc convert typesafety 😎 Apr 21, 2026

SabineSIL changed the title ~~feat(developer):kmc convert typesafety 😎~~ feat(developer): kmc convert typesafety 😎 Apr 21, 2026

SabineSIL and others added 4 commits April 21, 2026 16:44

feat(developer): baseUrl problem of tsconfig.json, errorMsg typo

417c044

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

99892e3

…vert-Typesafety

feat(developer): restore tsconfig.json

4716887

Merge branch 'feat/developer/kmc-convert-Typesafety' of https://githu…

2edf63c

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety

SabineSIL marked this pull request as ready for review April 21, 2026 15:44

keyman-server modified the milestones: A19S27, A19S28 Apr 24, 2026

SabineSIL and others added 4 commits May 4, 2026 18:59

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

16e0570

…vert-Typesafety

feat(developer): add returntypes

1d3d53e

feat(developer): add tests and comments

9675611

feat(developer): add testfile

302e7b0

keyman-server modified the milestones: A19S28, A19S29 May 11, 2026

SabineSIL moved this from Todo to Code Review in Keyman May 18, 2026

SabineSIL and others added 5 commits May 19, 2026 09:57

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

eb5ac1e

…vert-Typesafety

feat(developer): add missing bracket

5052581

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

b72e98e

…vert-Typesafety

feat(developer): dummy change to start team city

82c8af2

ermshiperete reviewed May 22, 2026

View reviewed changes

keyman-server modified the milestones: A19S29, A19S30 May 23, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(developer): kmc convert typesafety 😎#15860

feat(developer): kmc convert typesafety 😎#15860
SabineSIL wants to merge 18 commits into
feat/developer/kmc-convertfrom
feat/developer/kmc-convert-Typesafety

SabineSIL commented Apr 20, 2026 •

edited

Loading

Uh oh!

keymanapp-test-bot Bot commented Apr 20, 2026 •

edited

Loading

Uh oh!

ermshiperete left a comment •

edited

Loading

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

ermshiperete May 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	const result = sut.run(inputFilename, undefined);
	const result = await sut.run(inputFilename, undefined);

	output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '' as string,
	output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '',

Uh oh!

Conversation

SabineSIL commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keymanapp-test-bot Bot commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

ermshiperete left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SabineSIL commented Apr 20, 2026 •

edited

Loading

keymanapp-test-bot Bot commented Apr 20, 2026 •

edited

Loading

ermshiperete left a comment •

edited

Loading